[Multicast] Add Multicast statistics API #3354

ceclinux · 2022-02-24T02:46:34Z

This PR adds a field Multicast for control-plane API NodeStatsSummary
and field MulticastGroup for public statistics API group.

Signed-off-by: ceclinux [email protected]

For #3294

codecov-commenter · 2022-02-24T03:20:07Z

Codecov Report

Merging #3354 (594715f) into main (3a51abe) will decrease coverage by 21.32%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #3354       +/-   ##
===========================================
- Coverage   63.38%   42.06%   -21.33%     
===========================================
  Files         280      325       +45     
  Lines       39750    51694    +11944     
===========================================
- Hits        25197    21745     -3452     
- Misses      12593    28330    +15737     
+ Partials     1960     1619      -341

Flag	Coverage Δ
integration-tests	`38.22% <ø> (?)`
kind-e2e-tests	`?`
unit-tests	`43.73% <ø> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/apis/controlplane/types.go	`0.00% <ø> (ø)`
pkg/log/log.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/default.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/util/runtime/runtime.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/openflow/groups.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/ovs/openflow/ofctrl_pipeline.go	`0.00% <0.00%> (-95.84%)`	⬇️
pkg/ovs/openflow/logs.go	`10.34% <0.00%> (-89.66%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`0.00% <0.00%> (-85.30%)`	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	`0.00% <0.00%> (-84.45%)`	⬇️
pkg/apis/controlplane/register.go	`0.00% <0.00%> (-83.34%)`	⬇️
... and 313 more

wenyingd

One question: why adding MulticastGroupMember/MulticastGroupMemberList in both pkg/apis/stats/types.go and pkg/apis/stats/v1alpha1/types.go? I think adding in pkg/apis/stats/v1alpha1/types.go is enough.

wenyingd · 2022-03-10T07:34:20Z

pkg/apis/controlplane/types.go

+	MulticastStats []MulticastStats
+}
+
+// MulticastStats contains the information for Multicast Groups that a pod has joined.


s/MulticastStats contains the information for Multicast Groups that a pod has joined/MulticastStats contains the multicast groups that a Pod has joined/s

ceclinux · 2022-03-22T01:24:25Z

One question: why adding MulticastGroupMember/MulticastGroupMemberList in both pkg/apis/stats/types.go and pkg/apis/stats/v1alpha1/types.go? I think adding in pkg/apis/stats/v1alpha1/types.go is enough.

From the @tnqn reply in #1221

@ceclinux please take a look at https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md which explains why an internal version is required. Actually it's not a must right now but will be needed once we have another API version.

After reading the docs provided, I think we should have the internal structs changes(/stats/v1alpha1/types.go) and versioned api changes(pkg/apis/stats/v1alpha1/types.go) in this pr.

ceclinux · 2022-03-22T02:13:23Z

/test-multicast-e2e

pkg/apis/controlplane/v1beta2/types.go

tnqn · 2022-04-26T03:47:08Z

pkg/apis/stats/types.go

+// MulticastGroupMember contains the mapping between Multicast Group and Pods.
+type MulticastGroupMember struct {
+	metav1.TypeMeta
+	metav1.ObjectMeta


If we use standard K8s API for this, what would be the name of the object?

I think the the name of the object would be same as group IP

Also think it is not necessary to have a single field for name also namespace.

wenyingd · 2022-04-26T04:09:11Z

pkg/apis/controlplane/v1beta2/types.go

+	Pod *PodReference `json:"pod,omitempty" protobuf:"bytes,1,opt,name=pod"`
+
+	// List of multicast IPs that the pod has joined.
+	GroupJoinedList []IPAddress `json:"groupJoinedList,omitempty" protobuf:"bytes,2,rep,name=groupJoinedList"`


Maybe rename it "GroupList"?

Updated. The whole struct has changed.

wenyingd · 2022-04-26T04:16:40Z

pkg/apis/controlplane/v1beta2/types.go

+}
+
+// MulticastStats contains the multicast groups that a Pod has joined.
+type MulticastStats struct {


It looks this struc is only for the group list per Pod? But the public API is shown pod list per group in MulticastGroupMember. Why use a different orgnization here?

Do you plan to show the multicast traffic stats in this API on the scenario no ANP is applied to the Pod?

It looks this struc is only for the group list per Pod? But the public API is shown pod list per group in MulticastGroupMember. Why use a different orgnization here?

Updated. Please check #3354 (comment)

Do you plan to show the multicast traffic stats in this API on the scenario no ANP is applied to the Pod?

No sure I understand your question. the group pods relation will be affected by multicast np. If igmp is block by np, the pod is not considered joined to the group in the final antctl get multicaststats representation. no ANP and ANP with allow action to the igmp are considered to be the same.

I mean do you want to show the packet number in the MulticastStats as you only list the Pod count of a Group in this struct? Or the the multicast packet numbers are only shown in the result of ANP stats? But if a Multicast group is not applied with any ANP, we can't get the traffic numbers.

If you want to check traffic statistics not covered by ANP, you can check the command antctl get multicaststats running in antrea-agent, as described here #3294.

wenyingd · 2022-04-26T04:23:11Z

pkg/apis/controlplane/v1beta2/types.go

@@ -325,6 +325,16 @@ type NodeStatsSummary struct {
 	AntreaClusterNetworkPolicies []NetworkPolicyStats `json:"antreaClusterNetworkPolicies,omitempty" protobuf:"bytes,3,rep,name=antreaClusterNetworkPolicies"`
 	// The TrafficStats of Antrea NetworkPolicies collected from the Node.
 	AntreaNetworkPolicies []NetworkPolicyStats `json:"antreaNetworkPolicies,omitempty" protobuf:"bytes,4,rep,name=antreaNetworkPolicies"`
+	// List of PodMulticast statistics collected from the Node.


I think Multicast stats API should start from version v1alpha1 if you don't change the feature gate version.

Besides, does this new kind need to add into the function addConversionFuncs https://github.com/antrea-io/antrea/blob/main/pkg/apis/controlplane/v1beta2/conversion.go#L28

I think Multicast stats API should start from version v1alpha1 if you don't change the feature gate version.

I think the the API changed I made at controlplane in this pr fit the case described here Adding Unstable Features to Stable Versions https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions

Besides, does this new kind need to add into the function addConversionFuncs https://github.com/antrea-io/antrea/blob/main/pkg/apis/controlplane/v1beta2/conversion.go#L28

I checked the auto-generated func Convert_v1beta2_MulticastStats_To_controlplane_MulticastStats and Convert_controlplane_MulticastStats_To_v1beta2_MulticastStats, both LGTM. Could you explain why we need add addConversionFuncs? Actually addConversionFuncs confuses me because delete the one kind from this func doesn't affect the generated code.

@tnqn Could you take a look at these questions? Not sure my description was correct.

I think you could follow https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-field-in-existing-api-version?
The field could be added to the struct, but whether it will be collected, filled, and consumed depends on whether MulticastStats feature gate is enabled.
If we want to reuse the NodeStatsSummary API to report multicast stats data, we cannot really have another version starting from v1alpha1.

tnqn

LGTM overall

tnqn · 2022-05-11T06:46:36Z

pkg/apis/controlplane/v1beta2/types.go

@@ -342,6 +342,16 @@ type NodeStatsSummary struct {
 	AntreaClusterNetworkPolicies []NetworkPolicyStats `json:"antreaClusterNetworkPolicies,omitempty" protobuf:"bytes,3,rep,name=antreaClusterNetworkPolicies"`
 	// The TrafficStats of Antrea NetworkPolicies collected from the Node.
 	AntreaNetworkPolicies []NetworkPolicyStats `json:"antreaNetworkPolicies,omitempty" protobuf:"bytes,4,rep,name=antreaNetworkPolicies"`
+	// List of PodMulticast statistics collected from the Node.


nit: could we make the comment consistent with the one in pkg/apis/controlplane/types.go?

tnqn · 2022-05-11T06:57:50Z

pkg/apis/stats/v1alpha1/types.go

@@ -85,6 +85,34 @@ type NetworkPolicyStats struct {
 	TrafficStats TrafficStats `json:"trafficStats,omitempty" protobuf:"bytes,2,opt,name=trafficStats"`
 }

+// +genclient
+// +resourceName=multicastgroupmembers


nit: this is not required. It was added to xxxstats struct because its plural automatically generated is not correct.

tnqn · 2022-05-11T07:02:26Z

pkg/apis/stats/v1alpha1/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// MulticastGroupMember contains the mapping between Multicast Group and Pods.
+type MulticastGroupMember struct {


Do we plan to have more information for MulticastGroup that can be exposed by this API?
If no, current API name and group work for me.
If yes, I wonder if we should just use single API for all information specific to MulticastGroup, name it MulticastGroup.

No, we don't have plan to expose more group-level multicast information.

It should be called MulticastGroupMembers, but I also feel a more generic name like MulticastGroup or MulticastGroupInfo is better for future extensions.

Updated.

Changed struct name to MulticastGroup. Trade explicitness for extensibility seems to be a good deal for me.

ceclinux · 2022-05-11T23:31:01Z

Currently, my implementation is antrea-agents sending whole multicast stats to the controller, which aligns with the control plane API change in the pr.
I am thinking about whether I should shift implementation to an incremental style for multicast stats, maybe something like

// MulticastStats contains the multicast groups that a Pod has joined.
type MulticastStats struct {
	// Group is the IP of Multicast Group.
	Group string `json:"group,omitempty" protobuf:"bytes,1,opt,name=group"`
	// PodsPatches is the list of MulticastGroupPatch that describes changes of pod members.
	PodsPatches []MulticastGroupPatch `json:"podsPatches,omitempty" protobuf:"bytes,2,rep,name=podsPatches"`
}

// MulticastGroupPatch describes the incremental update for the pod members with a group.
type MulticastGroupPatch struct {
	AddedGroupMembers   []PodReference `protobuf:"bytes,1,rep,name=addedGroupMembers"`
	RemovedGroupMembers []PodReference `protobuf:"bytes,2,rep,name=removedGroupMembers"`
}

pros and cons:

whole multicast stats style: easier, may hit performance issue.
incremental style: harder to code, need to take extra effort to solve antrea-agent restart and antrea-controller restart issue, better performance.

@tnqn @wenyingd @liu4480

wenyingd · 2022-05-12T01:32:34Z

pros and cons:

whole multicast stats style: easier, may hit performance issue.

incremental style: harder to code, need to take extra effort to solve antrea-agent restart and antrea-controller restart issue, better performance.

@tnqn @wenyingd @liu4480

I prefer agent to send the full update ( containing all latest statistics ) every time.

tnqn · 2022-05-12T03:33:11Z

Incremental style may not just work: antrea-controller doesn't persist data, using incremental update will also introduce the problem when it's time to do a full sync (how does agent know antrea-controller restarts).

A periodical sync of a list of Pods that have joined a group seems fine to me. There were only 110 Pods on a Node at most by K8s default configuration, even they are all for multicast, the data was just few KBs per group, considering each Pod object in K8s also has few KBs.

antoninbas

overall, it's a bit strange to me that we use the stats terminology and the existing stats API to report multicast group membership information

antoninbas · 2022-05-12T17:17:44Z

pkg/apis/controlplane/types.go

+
+// MulticastStats contains the multicast groups that a Pod has joined.
+type MulticastStats struct {
+	// Group is the IP of Multicast Group.


Suggested change

// Group is the IP of Multicast Group.

// Group is the IP of the multicast group.

antoninbas · 2022-05-12T17:17:58Z

pkg/apis/controlplane/types.go

+type MulticastStats struct {
+	// Group is the IP of Multicast Group.
+	Group string
+	// Pods is the list of Pods that has joined the multicast group.


Suggested change

// Pods is the list of Pods that has joined the multicast group.

// Pods is the list of Pods that have joined the multicast group.

antoninbas · 2022-05-12T17:19:38Z

pkg/apis/controlplane/types.go

+	Multicast []MulticastStats
+}
+
+// MulticastStats contains the multicast groups that a Pod has joined.


Suggested change

// MulticastStats contains the multicast groups that a Pod has joined.

// MulticastStats contains the list of Pods that has joined a multicast group, for a given Node.

antoninbas · 2022-05-12T17:21:54Z

pkg/apis/controlplane/types.go

+	// The stats related to multicast collected from the Node.
+	Multicast []MulticastStats


For me stats implies things like counters. Right now it seems that we only provide group membership information. Is that going to change in the future? If not, this should probably be renamed. @jianjuns @tnqn

If I understand it correctly, you were implying MulticastStats should not be put in NodeStatsSummary API because it doesn't include real stats(only group-pod membership) for now. It should be added to another controlplane API.

For me creating a new API may seem like overkill. Also, We may add extra stats-like fields in this struct in the future.

I think Antonin means the struct should not be named as MulticastStats as it has nothing to do with stats. If there is no plan to add statistics to the struct in future, probably name it MulticastGroupInfo.

I think using the same API (NodeStatsSummary) is good, even though in a future version maybe the API should be renamed to something more generic (e.g., NodeSummary). But yes, you don't have any stats in MulticastStats at the moment, and MulticastGroupInfo sounds like a more appropriate name.

Updated. Thanks for the explanation.

jianjuns · 2022-05-15T17:56:14Z

pkg/apis/controlplane/types.go

@@ -340,6 +340,16 @@ type NodeStatsSummary struct {
 	AntreaClusterNetworkPolicies []NetworkPolicyStats
 	// The TrafficStats of Antrea NetworkPolicies collected from the Node.
 	AntreaNetworkPolicies []NetworkPolicyStats
+	// The stats related to multicast collected from the Node.


Revise the comments too: The stats ... -> Multicast group information

Updated. Thanks

jianjuns

Nits.

jianjuns · 2022-05-16T17:24:18Z

pkg/apis/controlplane/types.go

@@ -340,6 +340,16 @@ type NodeStatsSummary struct {
 	AntreaClusterNetworkPolicies []NetworkPolicyStats
 	// The TrafficStats of Antrea NetworkPolicies collected from the Node.
 	AntreaNetworkPolicies []NetworkPolicyStats
+	// Multicast group information related to multicast collected from the Node.


I think we should remove "related to multicast"

jianjuns · 2022-05-16T17:24:40Z

pkg/apis/controlplane/v1beta2/types.go

+	Multicast []MulticastGroupInfo `json:"multicast,omitempty" protobuf:"bytes,5,rep,name=multicast"`
+}
+
+// MulticastGroupInfo contains the list of Pods that has joined a multicast group, for a given Node.


has -> have

jianjuns · 2022-05-16T17:25:32Z

pkg/apis/stats/v1alpha1/types.go

+// +genclient:nonNamespaced
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// MulticastGroup contains the mapping between Multicast Group and Pods.


I feel no need to capitalize "Multicast Group".

jianjuns

LGTM. Others might have more comments.

antoninbas

LGTM as well

tnqn · 2022-05-18T06:34:36Z

/test-all

tnqn · 2022-05-18T09:55:49Z

/test-e2e

tnqn · 2022-05-18T15:15:33Z

@ceclinux I just realized the commit message is out of date, please update the PR description with correct one, I will use it when merging the commit.

This PR adds a field Multicast for control-plane API NodeStatsSummary and field MulticastGroup for public statistics API group. Signed-off-by: ceclinux <[email protected]>

ceclinux · 2022-05-18T15:34:10Z

@ceclinux I just realized the commit message is out of date, please update the PR description with correct one, I will use it when merging the commit.

updated.

tnqn · 2022-05-18T15:42:10Z

/skip-all

ceclinux changed the title ~~Add Multicast statistics API and controlplane stats API~~ [Multicast] Add Multicast statistics API and controlplane stats API Feb 24, 2022

ceclinux changed the title ~~[Multicast] Add Multicast statistics API and controlplane stats API~~ [Multicast] Add Multicast statistics API Feb 24, 2022

ceclinux force-pushed the multicast_stats_api branch 2 times, most recently from e2b7ec3 to 2291d90 Compare February 24, 2022 03:01

ceclinux requested review from tnqn and wenyingd February 24, 2022 07:52

ceclinux mentioned this pull request Feb 24, 2022

Support Multicast statistics #3294

Closed

ceclinux force-pushed the multicast_stats_api branch 2 times, most recently from 149a55b to 949db7e Compare March 8, 2022 13:36

wenyingd reviewed Mar 10, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch from 949db7e to abd415d Compare March 22, 2022 02:10

ceclinux mentioned this pull request Mar 23, 2022

Multicast Support in Antrea #2251

Closed

12 tasks

ceclinux requested a review from liu4480 April 13, 2022 02:52

ceclinux added this to the Antrea v1.7 release milestone Apr 24, 2022

ceclinux requested a review from wenyingd April 26, 2022 02:10

tnqn reviewed Apr 26, 2022

View reviewed changes

wenyingd reviewed Apr 26, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch 3 times, most recently from 8c3e41d to 0ade78f Compare April 27, 2022 09:27

ceclinux requested review from tnqn and wenyingd April 27, 2022 09:28

ceclinux force-pushed the multicast_stats_api branch from 0ade78f to 2414b35 Compare April 27, 2022 10:03

tnqn reviewed May 11, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch from 2414b35 to fadf317 Compare May 11, 2022 23:23

tnqn added api-review Categorizes an issue or PR as actively needing an API review. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API. action/release-note Indicates a PR that should be included in release notes. labels May 12, 2022

tnqn requested review from antoninbas and jianjuns May 12, 2022 04:12

antoninbas reviewed May 12, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch 2 times, most recently from 403c2c6 to 1e0024f Compare May 15, 2022 09:27

jianjuns reviewed May 15, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch from 1e0024f to 04b344c Compare May 16, 2022 07:24

jianjuns reviewed May 16, 2022

View reviewed changes

ceclinux force-pushed the multicast_stats_api branch from 04b344c to f75d166 Compare May 17, 2022 02:11

jianjuns previously approved these changes May 17, 2022

View reviewed changes

antoninbas previously approved these changes May 17, 2022

View reviewed changes

ceclinux requested a review from tnqn May 18, 2022 01:57

ceclinux dismissed stale reviews from antoninbas and jianjuns via ee0d87c May 18, 2022 06:27

ceclinux force-pushed the multicast_stats_api branch from f75d166 to ee0d87c Compare May 18, 2022 06:27

tnqn previously approved these changes May 18, 2022

View reviewed changes

[Multicast] Add Multicast statistics API and controlplane stats API

594715f

This PR adds a field Multicast for control-plane API NodeStatsSummary and field MulticastGroup for public statistics API group. Signed-off-by: ceclinux <[email protected]>

ceclinux dismissed tnqn’s stale review via 594715f May 18, 2022 15:32

ceclinux force-pushed the multicast_stats_api branch from ee0d87c to 594715f Compare May 18, 2022 15:32

tnqn merged commit c23a20c into antrea-io:main May 18, 2022

	// Group is the IP of Multicast Group.
	// Group is the IP of the multicast group.

	// Pods is the list of Pods that has joined the multicast group.
	// Pods is the list of Pods that have joined the multicast group.

	// MulticastStats contains the multicast groups that a Pod has joined.
	// MulticastStats contains the list of Pods that has joined a multicast group, for a given Node.

		// The stats related to multicast collected from the Node.
		Multicast []MulticastStats

[Multicast] Add Multicast statistics API #3354

[Multicast] Add Multicast statistics API #3354

Conversation

ceclinux commented Feb 24, 2022 • edited Loading

codecov-commenter commented Feb 24, 2022 • edited Loading

Codecov Report

wenyingd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux commented Mar 22, 2022

ceclinux commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceclinux commented May 11, 2022 • edited Loading

wenyingd commented May 12, 2022

tnqn commented May 12, 2022

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

tnqn commented May 18, 2022

tnqn commented May 18, 2022

tnqn commented May 18, 2022

ceclinux commented May 18, 2022

tnqn commented May 18, 2022

ceclinux commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Feb 24, 2022 •

edited

Loading

ceclinux Apr 26, 2022 •

edited

Loading

ceclinux Apr 27, 2022 •

edited

Loading

ceclinux commented May 11, 2022 •

edited

Loading